Jae/rangeprooffix #75

jaekwon · 2018-07-02T05:40:19Z

0.9.0 (July 1, 2018)

BREAKING CHANGES

RangeProof.VerifyItem doesn't require an index.
Only return values in range when getting proof.
Return keys as well.

BUG FIXES

traversal bugs in traverseRange.

Merging to develop early since the previous version is just broken.

liamsi

Left a few comments about documentation and exported methods.
LGTM in general. develop needs to be merged in or rebased upon.

liamsi · 2018-07-02T14:47:01Z

proof_range.go

+// `i` is the relative index within this RangeProof.
+// Also see LeftIndex().
+// Verify that a key has some value.
+// Does not assume that the proof itself is valid, call Verify() first.


If this is always intended to be used in combination with Verify(), then it would safer if we do not expose this functionality. In other words: Is there a use-case where we need to call VerifyItem(...) without Verify()?

You might want to call Verify without VerifyItem. If we're going to have a Verify() method, might as well have a separate VerifyItem method. We return an error if Verify() wasn't called first.

This isn't true here but it is true here now: #78
In multi-layered merkle proofs, say, it is more efficient to not have the intermediate root hashes in the layered merkle proofs, so in that case you'll want to verify the leaves, but compute the root hash and override the root hash verification requirement by calling:

proof.Verify(proof.ComputeRootHash())

which is cheap due to memoization (in the linked PR) and enables the other verification functionality.

liamsi · 2018-07-02T14:49:07Z

proof_range.go

@@ -236,24 +264,35 @@ func (proof *RangeProof) _verify(root []byte) (treeEnd bool, err error) {
 // If keyStart or keyEnd don't exist, the leaf before keyStart
 // or after keyEnd will also be included, but not be included in values.
 // If keyEnd-1 exists, no later leaves will be included.
+// If keyStart <= keyEnd and both not nil, panics.


Shouldn't this be keyStart >= keyEnd?

liamsi · 2018-07-02T15:06:44Z

proof_range.go

@@ -20,8 +21,21 @@ type RangeProof struct {
 	treeEnd int // 0 if not set, 1 if true, -1 if false.
 }

+func (proof *RangeProof) Keys() (keys [][]byte) {


This is only used in the test below so far. Do you assume this will be useful for other purposes, too? As far as I can see, the keys are always returned (now) together with the range-proof (not sure why we need an additional public Keys() method).

I think so yeah, the keys returned are the queried keys. the .Keys() in a RangeProof are all the keys for values queried + any other keys like the item before startKey if startKey is provided (not nil) and doesn't exist in the iavl store, so they're different in functionality. I'll add a comment saying so.

// Keys returns all the keys in the RangeProof. NOTE: The keys here may // include more keys than provided by tree.GetRangeWithProof or // VersionedTree.GetVersionedRangeWithProof. The keys returned there are only // in the provided [startKey,endKey){limit} range. The keys returned here may // include extra keys, such as: // - the key before startKey if startKey is provided and doesn't exist; // - the key after a queried key with tree.GetWithProof, when the key is absent. func (proof *RangeProof) Keys() (keys [][]byte) {

Thanks for the explanation. That makes sense.

liamsi · 2018-07-02T15:18:18Z

proof_range.go

+	return proof.LeftPath.Index()
+}
+
+// `i` is the relative index within this RangeProof.


Not sure we need to explain what i is here? It is not a parameter/argument nor do we return it. It looks like an implementation detail. Looks like this comment belongs inside of the method.

Removed in the new PR.

liamsi · 2018-07-02T15:20:58Z

proof_test.go

+		{start: 0xf8, end: 0x00, pnc: true},                                  // #19
+		{start: 0x10, end: 0x10, pnc: true},                                  // #20
+		{start: 0x12, end: 0x12, pnc: true},                                  // #21
+		{start: 0xff, end: 0xf7, pnc: true},                                  // #22


exemplary testing 👍

jaekwon · 2018-07-02T22:10:57Z

I pushed fixes to #78

liamsi · 2018-07-02T23:59:27Z

Reviewed #78. LGTM

0.9.1 -- compute hash from rangeproof

liamsi · 2018-07-03T01:30:21Z

Merged #78 and resolved conflicts to develop. Will merge if ci goes through.

* VerifyItem takes no index; Return keys/values in range; Fix * compute hash from rangeproof * Require Verify() before Verify*() * Review fixes from #75 * Bump version, update changelog

0.9.1 -- compute hash from rangeproof Require Verify() before Verify*() Review fixes from cosmos#75 First commit for generalized merkle proofs Fix imports Bump version to 0.9.3 Return nil on empty tree - bump version to 0.10.0 Add OpDecoders fix test finalize rebase

0.9.1 -- compute hash from rangeproof Require Verify() before Verify*() Review fixes from cosmos#75 First commit for generalized merkle proofs Fix imports Bump version to 0.9.3 Return nil on empty tree - bump version to 0.10.0 Add OpDecoders fix test finalize rebase fix test

0.9.1 -- compute hash from rangeproof Require Verify() before Verify*() Review fixes from cosmos#75 First commit for generalized merkle proofs Fix imports Bump version to 0.9.3 Return nil on empty tree - bump version to 0.10.0 Add OpDecoders fix test finalize rebase fix test fix toml

* 'develop' of github.com:danil-lashin/iavl: Prep Release 0.11.0 (cosmos#111) Remove architecture dependent getter functions (cosmos#96) Use 8 bytes to store int64 components of database keys (cosmos#107) Update to CircleCI 2.0 (cosmos#108) Release 0.10.0: Update Changelog and bump version (cosmos#99) delete empty file (relict from merging master into develop) Mutable/Immutable refactor and GetImmutable snapshots (cosmos#92) Remove unreachable code Remove unused variable dep: Change tendermint dep to be ^v0.22.0 (cosmos#91) Fix benchmark scripts for current code (cosmos#89) release v0.9.2 (cosmos#82) Various lints (cosmos#80) Jae/rangeprooffix (cosmos#75)

* VerifyItem takes no index; Return keys/values in range; Fix * Bump version * 0.9.1 -- compute hash from rangeproof * Require Verify() before Verify*() * Review fixes from cosmos#75

* VerifyItem takes no index; Return keys/values in range; Fix * compute hash from rangeproof * Require Verify() before Verify*() * Review fixes from cosmos#75 * Bump version, update changelog

0.9.1 -- compute hash from rangeproof Require Verify() before Verify*() Review fixes from cosmos#75 First commit for generalized merkle proofs Fix imports Bump version to 0.9.3 Return nil on empty tree - bump version to 0.10.0 Add OpDecoders fix test finalize rebase fix test fix toml

jaekwon added 2 commits July 1, 2018 22:35

VerifyItem takes no index; Return keys/values in range; Fix

21222ae

Bump version

13b04ba

jaekwon requested review from liamsi and UnitylChaos July 2, 2018 05:40

liamsi approved these changes Jul 2, 2018

View reviewed changes

jaekwon added 3 commits July 2, 2018 14:43

0.9.1 -- compute hash from rangeproof

4fd49ed

Require Verify() before Verify*()

48ba598

Review fixes from #75

647806e

liamsi added 2 commits July 3, 2018 02:23

Merge pull request #78 from tendermint/jae/rangeproof_computehash

5450e55

0.9.1 -- compute hash from rangeproof

Merge branch 'develop' into jae/rangeprooffix

e286d69

liamsi merged commit 0b6f4eb into develop Jul 3, 2018

liamsi added a commit that referenced this pull request Jul 3, 2018

Release/v0.9.1 (#79)

4ed3e87

* VerifyItem takes no index; Return keys/values in range; Fix * compute hash from rangeproof * Require Verify() before Verify*() * Review fixes from #75 * Bump version, update changelog

tac0turtle deleted the jae/rangeprooffix branch July 11, 2019 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jae/rangeprooffix #75

Jae/rangeprooffix #75

jaekwon commented Jul 2, 2018

liamsi left a comment •

edited

Loading

liamsi Jul 2, 2018

jaekwon Jul 2, 2018

jaekwon Jul 2, 2018

liamsi Jul 2, 2018

jaekwon Jul 2, 2018

liamsi Jul 2, 2018 •

edited

Loading

jaekwon Jul 2, 2018

jaekwon Jul 2, 2018

liamsi Jul 3, 2018

liamsi Jul 2, 2018

jaekwon Jul 2, 2018

liamsi Jul 2, 2018

jaekwon commented Jul 2, 2018

liamsi commented Jul 2, 2018

liamsi commented Jul 3, 2018

Jae/rangeprooffix #75

Jae/rangeprooffix #75

Conversation

jaekwon commented Jul 2, 2018

0.9.0 (July 1, 2018)

liamsi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaekwon commented Jul 2, 2018

liamsi commented Jul 2, 2018

liamsi commented Jul 3, 2018

liamsi left a comment •

edited

Loading

liamsi Jul 2, 2018 •

edited

Loading